The map should be a const & for the API.

The map should be a const & for the API.

Can you please declare these functions in the same order as the header? It makes it easier to navigate.

Also the 2 argument constructor with the speakFeature can now call the new constructor with an empty map. Then the redundant if statement can be removed. It's best to keep the copy and paste code to a minimum to improve the maintenance.

Can you please declare these functions in the same order as the header? It makes it easier to navigate.

Also the 2 argument constructor with the SpeakableString can now call the new constructor with an empty map. Then the copied code can be removed. It's best to keep the copy and paste code to a minimum to improve the maintenance.

Sure I would keep that in mind.
Being a fresher I wasn't aware.

C++ API is not allowed in C API. Please see ipron_createWithDefaults for how to pass in such values. That API also ensures that the 2 lengths are exactly the same.

For consistency with the PronounConcept API, it would be ideal if this was also named iinf_createWithDefaults to be consistent with the naming.

This internal C++ header should not be used in public C API.

I will fix that

I don't think that this file needs to change.

Yup sure, I mistakenly added a line and forgot

These tests look reasonable.

Let me correct my comment. The structure looks generally good, but the last test is wrong. The initial gender is feminine. So the gender result should be feminine too. This looks like a copy and paste error.

@grhoten
Was this the expected behavior in this function? If not, could you please point out where I am going wrong?

No. Within the DictionaryLookupFunction, you're only returning the grammeme value. This does not need to change.

DictionaryLookupFunction::getFeatureValue likely needs to be updated to query the relevant values from displayValue. It should probably query DisplayValue::getFeatureValue or DisplayValue::getConstraintMap

Okay sure I wanted to know exactly what I need to do, based on your previous comment I came up with this commit, could you please tell what is expected to make the tests pass so that i could implement the same and further add tests in InfectableStringConceptTest-c.cpp as well to complete this task ?

Can you please revert the change to this file. It looks like it's only changing the empty lines.

These changes are confusing. Why are these changes needed?

More precisely, why is getFeatureValue being called here? Why is the feature name being checked for gender? What's special about gender or number? Why not grammatical case?

auto displayConstraint = displayConstraintMap.find(feature);
if (displayConstraint != displayConstraintMap.end()) {
    auto numberFeature = npc(getModel())->getFeature(u"number");
    if (feature.getName() == u"gender" && baseValue && numberFeature != nullptr
        && displayConstraintMap.find(*npc(numberFeature)) != displayConstraintMap.end()) {
        return baseValue.release();
    }
    return new SpeakableString(displayConstraint->second);
} 

This is the test case which was failing before the changes:

<test>
  <source definiteness="definite">cometa</source>
  <initial gender="masculine"/>
  <result gender="masculine" number="singular">el cometa</result>
</test>

• InflectableStringConcept::getFeatureValue asks the default display function for a rendered DisplayValue, so feature lookups reflect whatever the display layer injected (articles, inflections, etc.). Spanish determiners add both gender and number to that map, and the gender can contradict the lexeme default (la cometa).

• To avoid reporting the article’s guess as the noun’s gender, the code detects feature == "gender" and, when the number constraint is present too, returns the lexeme’s base value instead of the display-layer value. English articles never set gender, and no language mutates case in this path yet (I am not sure though), so only the gender/number pair needs a guard today.

• InflectableStringConcept::getFeatureValue asks the default display function for a rendered DisplayValue, so feature lookups reflect whatever the display layer injected (articles, inflections, etc.).

• Spanish determiners add both gender and number to that map, and the gender can contradict the lexeme default (la cometa).
  To avoid reporting the article’s guess as the noun’s gender, the code detects feature == "gender" and, when the number constraint is present too, returns the lexeme’s base value instead of the display-layer value, as these conditions when satisfied point to the case when we have an article being inserted.

• Grammatical case isn’t treated specially because the Spanish article logic never synthesizes case information—so there’s no regression to guard against. If a language later injected case-altering determiners we’d need a similar safeguard, but today only the gender/number pair exhibits the mismatch.

This behavior does not scale, and it does not make sense in my mind. An InflectableStringConcept is just a simplified SemanticConcept. A SemanticConcept should not have this issue. It does not have this kind of logic, and it does not check on specific grammatical category values.

Yes, you're right it is just a general constructor and logic, I think the underlying locale specific synthesizer needs to be addressed for the failing test case.

This seems like a good improvement.

Please split this line and use curly braces. It makes it easier to place break points.

Please change this calculation to use std::ssize instead. It's a C++20 feature.

Generally, I've tried to keep the first line the header for the current file, followed by an empty line, sort the headers, and leave npc.h last.

Can you please sort the headers in this section?

For future reference to self and other reviewers, the DisplayValue constructor with a SpeakableString takes care of the speak usage. It didn't disappear.

@grhoten
Yes, in the latest commit I did the following change in the InflectableStringConcept::getDisplayValue:
As their are initial constraints as well we might need to pass the constraintMap which was constructed into the defaultDisplayValue. I am not sure, please validate this step, Although the tests are passing.

If this step is correct i will resolve the macos test issue and remove the lines used for debugging so that the PR is ready to merge.

Please remove the logging changes with iostream. Please use Catch for logging any relevant information. The tests should run clean by default so that it's easier to find relevant information for failing tests.

Yes actually I just wanted to be sure that the changes I did to InflectableStringConcept::getDisplayValue are correct I mean using the constraintMap from defaultDisplayValue so that we use the initial constraints as well, please validate the overall changes to that function from all commits . And then I will do a final cleanup commit.

Everything else looks fine, except for the comments that I provided about an hour ago.

No need for whitespace after the semicolon.

Please don't leave commented out code in your changes. It makes it harder to read.

👍